- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 626
fix: Parsing priority for custom inline content and styles #2119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| The latest updates on your projects. Learn more about Vercel for GitHub. 
 | 
| @blocknote/ariakit
 @blocknote/code-block
 @blocknote/core
 @blocknote/mantine
 @blocknote/react
 @blocknote/server-util
 @blocknote/shadcn
 @blocknote/xl-ai
 @blocknote/xl-docx-exporter
 @blocknote/xl-email-exporter
 @blocknote/xl-multi-column
 @blocknote/xl-odt-exporter
 @blocknote/xl-pdf-exporter
 commit:  | 
| "content": [ | ||
| { | ||
| "styles": { | ||
| "backgroundColor": "transparent", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the "transparent"? or can we get rid of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For context, this is a result of us now being able to parse multiple styles from a single HTML element, as GDocs puts all text decorations into a single style attribute.
Right now, any style attribute on a span element with color/backgroundColor fields gets parsed as a TextColor/BackgroundColor style respectively, regardless of the value. I think it makes sense to filter out values like transparent or unset though, but this is out of scope for this PR - I think we should make a new issue for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewlipski can you make that issue since you've the most context on it?
| executeTest: testCopyHTML, | ||
| }, | ||
| { | ||
| testCase: { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@matthewlipski does this test also test pasting / parsing?
Are we confident the test that was added would have failed pre-fix? (and thus will catch potential regressions correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I reverted the other changes while keeping the test and it did indeed fail. It does also test copy/paste (the copy/paste equality tests also use the copy test cases).
It's not obvious but export/parse is also tested. We previously had tests for mentions, the reason they didn't catch this issue though was because the mentions were unstyled. After adding dom.style.backgroundColor = "red" to the mention DOM element in testSchema.ts, the export/parse equality test failed without the other changes from this PR.
Summary
This PR fixes an issue where default styles like text and background color were prioritised over custom inline content & styles.
This could be seen in the mentions inline content example, where, copying and pasting a mention would parse it as a background color style instead of a mention.
Closes #2064
Rationale
N/A
Changes
runsBeforeto custom inline content and style implementations.Impact
This PR should fix any potential issues that come from custom inline content and styles priority.
Testing
backgroundColorinline style, to replicate the issue fixed in this PR.Screenshots/Video
N/A
Checklist
Additional Notes